-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add check for undefined header in Column resizer #1156
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this doesn't show the error anymore, but it doesn't allow me to actually resize the column in the example the user sent in. Does it work for you in the storybook story?
@elephantcatdog the storybook story works.
The storybook story works-ish..? I'm not too confident about the expected behavior though: For user's example, I think it works too but because it makes the table width increase which cause the horizontal scroll appear while things look unchanged..? I was thinking that we disable resizing for the the selection/first column, should we do the same for column manager column? That would keep the table width the same too. But I'm not sure what the expected behavior should be. |
@LostABike Oooh, I see now. I was trying to make the column manager column bigger, but I actually don't see a reason for that to be bigger IMO. In that case, I tested it and this looks good to me. Except that there's a test failing, but it looks like it's a CSS visual test on tiles, so that's unrelated. I'm going to merge main in and rerun them to see if that fixes the test. Update: I ran the tests locally and it passed... so 🤷♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screen.Recording.2023-04-04.at.15.20.01.movI think it's something with how we calculate min width of the column based on table width. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can merge this as a hotfix for crashing. Table changing size is better than app not working.
We just need to add a new issue for resizing bugs.
@LostABike @mayank99 What do you think?
Yea I think that would be optimal...I could not figure the following bug out x_x |
Sounds good to me. Lets merge this PR and reopen the issue. |
Changes
When resizing the last column that is column manager in
expand
mode, the table crashes with error "cannot read properties of undefined" which seem to occur from usinggetHeaderWidth
function. So I added a check within that function to see whether ifheader
is undefined and return 0.Closes #1004
Testing
isResizable=true
andcolumnResizerMode='expand'
Docs